Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes error when reducing Fields with a condition on ImmersedBoundaryGrids #3440

Merged
merged 13 commits into from
Feb 1, 2024
Merged

Fixes error when reducing Fields with a condition on ImmersedBoundaryGrids #3440

merged 13 commits into from
Feb 1, 2024

Conversation

tomchor
Copy link
Collaborator

@tomchor tomchor commented Jan 24, 2024

This PR fixes an "ambiguous method" error that happens when trying to calculate an Average() (probably any reduction) with a given condition on ImmersedBoundaryGrids.

Thanks to @glwagner for helping me find the proper method definition.

Closes #3439

@tomchor
Copy link
Collaborator Author

tomchor commented Jan 24, 2024

The CPU tests all pass, but it seems like the GPU server isn't working? Does something need to be rebooted?

@glwagner
Copy link
Member

The CPU tests all pass, but it seems like the GPU server isn't working? Does something need to be rebooted?

Yeah, the GPU server is down right now, linked to construction happening at MIT. Hopefully it will be back up in a few days.

@tomchor tomchor changed the title Fixes error in whenreducing Fields with a condition on ImmersedBoundaryGrids Fixes error when reducing Fields with a condition on ImmersedBoundaryGrids Jan 24, 2024
@tomchor tomchor requested a review from glwagner January 29, 2024 17:04
tomchor and others added 2 commits January 29, 2024 15:02
@tomchor
Copy link
Collaborator Author

tomchor commented Jan 30, 2024

Tests are failing rn with an error I cannot reproduce:

Immersed Fields reduction [CPU]: Error During Test at /var/lib/buildkite-agent/builds/tartarus-3/clima/oceananigans/test/test_field_reductions.jl:223
  Got exception outside of a @test
  MethodError: no method matching arch_array(::CPU, ::BitArray{3})
  Closest candidates are:
    arch_array(::Distributed, ::Any)
     @ Oceananigans ~/builds/tartarus-3/clima/oceananigans/src/DistributedComputations/distributed_architectures.jl:263
    arch_array(::CPU, ::Array)
     @ Oceananigans ~/builds/tartarus-3/clima/oceananigans/src/Architectures.jl:59
    arch_array(::CPU, ::CuArray)
     @ Oceananigans ~/builds/tartarus-3/clima/oceananigans/src/Architectures.jl:60
    ...
  Stacktrace:
    [1] condition_operand
      @ ~/builds/tartarus-3/clima/oceananigans/src/ImmersedBoundaries/immersed_reductions.jl:26 [inlined]
    [2] sum(f::Function, c::Oceananigans.AbstractOperations.GridMetricOperation{Center, Center, Center, ImmersedBoundaryGrid{Float64, Periodic, Periodic, Bounded, RectilinearGrid{Float64, Periodic, Periodic, Bounded, Float64, Float64, Float64, OffsetVector{Float64, StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}}, OffsetVector{Float64, StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}}, OffsetVector{Float64, StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}}, CPU}, GridFittedBottom{Field{Center, Center, Nothing, Nothing, RectilinearGrid{Float64, Periodic, Periodic, Bounded, Float64, Float64, Float64, OffsetVector{Float64, StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}}, OffsetVector{Float64, StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}}, OffsetVector{Float64, StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}}, CPU}, Tuple{Colon, Colon, Colon}, OffsetArray{Float64, 3, Array{Float64, 3}}, Float64, FieldBoundaryConditions{BoundaryCondition{Oceananigans.BoundaryConditions.Periodic, Nothing}, BoundaryCondition{Oceananigans.BoundaryConditions.Periodic, Nothing}, BoundaryCondition{Oceananigans.BoundaryConditions.Periodic, Nothing}, BoundaryCondition{Oceananigans.BoundaryConditions.Periodic, Nothing}, Nothing, Nothing, BoundaryCondition{Flux, Nothing}}, Nothing, Oceananigans.Fields.FieldBoundaryBuffers{Nothing, Nothing, Nothing, Nothing, Nothing, Nothing, Nothing, Nothing}}, Oceananigans.ImmersedBoundaries.CenterImmersedCondition}, Nothing, CPU}, Float64, typeof(Vᶜᶜᶜ)}; condition::BitArray{3}, mask::Int64, dims::Tuple{Int64, Int64, Int64})

@simone-silvestri
Copy link
Collaborator

It's because there is no method for arch_array(arch, ::BitArray). I ll add it.

@tomchor tomchor requested a review from glwagner January 30, 2024 15:31
@tomchor
Copy link
Collaborator Author

tomchor commented Jan 30, 2024

It's because there is no method for arch_array(arch, ::BitArray). I ll add it.

Thanks @simone-silvestri. I wonder why I wasn't able to reproduce the error locally though. I even tested it on the GPU but everything passed somehow.

Seems like tests pass. I just need an approval and I'll merge.

simone-silvestri
simone-silvestri reviewed Jan 30, 2024
View reviewed changes
src/ImmersedBoundaries/immersed_reductions.jl
@@ -21,6 +21,13 @@ const IF = AbstractField{<:Any, <:Any, <:Any, <:ImmersedBoundaryGrid}
@inline condition_operand(func::Function, op::IF, ::Nothing, mask) = ConditionalOperation(op; func, condition=NotImmersed(truefunc), mask)
@inline condition_operand(func::typeof(identity), op::IF, ::Nothing, mask) = ConditionalOperation(op; func, condition=NotImmersed(truefunc), mask)

@inline function condition_operand(func::Function, op::IF, cond::AbstractArray, mask)
arch = architecture(op.grid)
arch_condition = arch_array(arch, cond)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we make this work also for fields, which are AbstractArrays, but do not have an arch_array method defined

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something like

Suggested change
arch_condition = arch_array(arch, cond)
arch_condition = arch_array(arch, parent(cond))

might suffice

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on_architecture was meant for this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something like

might suffice

I don't like that. Why not keep it as a Field?

@tomchor tomchor merged commit c3151d6 into main Feb 1, 2024
48 checks passed
@tomchor tomchor deleted the tc/immersed-reduction branch February 1, 2024 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simone-silvestri simone-silvestri simone-silvestri left review comments

@glwagner glwagner glwagner approved these changes

@navidcy navidcy Awaiting requested review from navidcy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ambiguous method error when calculating a conditional Average() with an ImmersedBoundaryGrid
3 participants
@tomchor @glwagner @simone-silvestri